Refactor integration testing system to build a dashboard and run in CI#44
Conversation
|
Preview of Generated dashboard in CI Once this is merged, future PRs will have a link posted automatically |
|
fyi, I get auth error for the dashboard link above. |
|
Yes sorry I need to fix the link |
astrofrog
changed the title
|
Here's the latest version of the dashboard: https://github.com/astropy/astropy-integration-testing/suites/68789126288/artifacts/6969081003 As noted now, the CI in PRs is just testing that the infrastructure is all working and packages are working at a basic level but only checks the first 10 tests. This is to save computing/waiting time in PRs. The main published version of the dashboard will be authoritative and include all tests. |
|
As a disclaimer, this was written in collaboration with an LLM - I thought this would be acceptable since it can produce much nicer dashboards than I ever could, and this is not going to be code running on users' machines. |
| return f"{pkg['pypi_name']}{_extras_suffix(pkg)} @ git+{repo}", None | ||
|
|
||
| else: | ||
| include_pre = variant == "pre" |
There was a problem hiding this comment.
Does it always run the pre job even if the versions are the same? If not can we hide it from the report?
There was a problem hiding this comment.
So technically pre applies to all packages including in wider non astropy ecosystem so could be useful to find out issues
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
Looks great!
Does this also means that we don't run into the duplicated conftest.py requirement any more, or tests are only passing for astroquery as you capped them at the first 10?
| # PR previews limit each package to the first 10 tests so the | ||
| # matrix finishes in minutes; conftest.py at the repo root reads | ||
| # this env var and truncates the collected items. | ||
| PYTEST_LIMIT_N: ${{ github.event_name == 'pull_request' && '10' || '' }} |
There was a problem hiding this comment.
shouldn't we make sure to run the whole test suite, at least for the coordinated packages?
There was a problem hiding this comment.
we discussed it off-github: this is not a concern of mine any more, having the limit on PRs is fine.
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
Merge this away; we discussed my concern off-band and it was made clear they are not concerns.
| pull_request: | ||
|
|
||
| permissions: | ||
| contents: write # to push gh-pages on main / schedule / dispatch |
There was a problem hiding this comment.
Should we add pull-requests: none just to be sure or it does not matter?
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
Thank you for this awesome upgrade! Some comments:
- Can we please pin GHA workflows to release hashes?
- Please add a cooldown to
dependabot.ymlwhile you are at it: https://docs.github.com/en/code-security/reference/supply-chain-security/dependabot-options-reference#cooldown- - As mentioned in other review, the dashboard preview link needs to be fixed
|
|
||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' |
There was a problem hiding this comment.
Shouldn't this use Python version set up in matrix above?
There was a problem hiding this comment.
No, this is just the version for the orchestrator, uv sets up the Python version it needs at the point of setting up the virtual environment to run the tests in.
There was a problem hiding this comment.
Comment added to clarify this
| ASTROPY_NIGHTLY_INDEX = "https://pypi.anaconda.org/astropy/simple" | ||
| LIBERFA_NIGHTLY_INDEX = ( |
There was a problem hiding this comment.
You mentioned interest to share this infrastructure with non-Astropy projects. If that is to happen, I suspect things like these need to be made customizable.
There was a problem hiding this comment.
Agree, though I'd rather keep this as a separate PR to make it easier to review, so I've opened a new issue here: #48
| pytest_args: ["-c", "sunpy_pytest.ini"] | ||
|
|
||
| # --- Affiliated packages auto-imported from | ||
| # https://github.com/astropy/astropy.github.com/blob/main/affiliated/registry.json |
There was a problem hiding this comment.
What about newer ones in https://www.astropy.org/affiliated/index.html#affiliated-packages-registry (from pyOpenSci cross-listing)?
There was a problem hiding this comment.
I think it would be clearer to future maintainers if you name the jobs explicitly rather than a vague "a fourth job" and so on.
|
The dashboard preview link will be fixed once this is merged and will work in subsequent PRs |
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
Add seven pyOpenSci-reviewed affiliated packages (astrodata, astromartini, autogalaxy, GALFITools, petrofit, stingray, zodipy) under a new pyopensci tier, ranked after affiliated in install and dashboard order. Set an explicit pull-requests: none workflow permission, document why setup-python uses a fixed version, and name the CI jobs explicitly in the README.
Pin every action in the integration and preview-link workflows to a full release commit SHA with a version comment. Add a 7-day cooldown to the github-actions Dependabot entry so a release has time to settle before a bump is proposed.
Done
Done
Will be fixed after this PR is merged |
|
@pllim - think I've addressed all your comments? |
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
✨ Main character energy. No cap! ✨
Given the preview link is currently broken, I will just have to trust your screenshot. We can fix it up later if your screenshot is no longer accurate.
p.s. Looks like this repo is no longer in-tox-icated. 😏
p.p.s. Webring is still on the table, right?
4 participants
Work in progress!